Fix focused notification sound playback - #1855

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
cmux	Ready	Preview, Comment	Mar 20, 2026 8:03am

No actionable comments were generated in the recent review. 🎉

sequenceDiagram
    participant Store as TerminalNotificationStore
    participant Defaults as UserDefaults / NotificationSoundSettings
    participant Sound as NSSound (system)
    participant Cmd as Shell / Custom Command

    Note over Store,Defaults: Notification arrives for terminal
    Store->>Store: determine shouldSuppressExternalDelivery
    alt suppressed
        Store->>Store: resolvedNotificationTitle(for:)
        Store->>Defaults: request selected sound / command
        Defaults->>Sound: playSelectedSound (via playSound)
        Sound-->>Store: playback finished (delegate) 
        Store->>Cmd: run configured custom command (with resolved title)
    else deliver externally
        Store->>SystemNotificationCenter: schedule user notification
    end

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Greptile Summary

This PR fixes two related bugs in focused-terminal notification handling: (1) it corrects a logical inversion (if !shouldSuppressExternalDelivery) that previously swallowed all notification actions when the app was focused on the originating terminal, and (2) it introduces an activePlaybackSounds retention map so that NSSound instances created for custom file playback are kept alive until the NSSoundDelegate didFinishPlaying callback fires — preventing sounds from being silently cut short.

Key changes:

• addNotification now routes to a new suppressedNotificationFeedbackHandler when external delivery is suppressed, instead of doing nothing
• playSuppressedNotificationFeedback calls NotificationSoundSettings.playSelectedSound() to give local audio feedback in the focused case
• playSoundFile(at:) retains the NSSound instance in a static activePlaybackSounds dictionary via a shared NSSoundDelegate, releasing it only after playback completes
• previewSound is refactored to share a common playSound private method with the new playSelectedSound entry point
• A new integration test verifies that suppressed notifications trigger local feedback handlers rather than external delivery handlers

Concern: playSuppressedNotificationFeedback discards the notification parameter (_ = notification) and never calls NotificationSoundSettings.runCustomCommand(...). scheduleUserNotification always fires this hook after scheduling; skipping it in the suppressed path means users with a custom notification command configured will never see it run when cmux is focused on the terminal that triggered the notification.

Confidence Score: 3/5

• Safe to merge for the sound-retention fix, but the missing runCustomCommand call in the suppressed path is a behavioral regression for users with custom notification commands configured.
• The core logic inversion fix and NSSound retention mechanism are both correct and well-tested. The new test provides good coverage for the suppressed-delivery path. Score is lowered because playSuppressedNotificationFeedback silently skips the runCustomCommand hook that the normal delivery path always fires — this is a real omission for users who rely on custom notification commands.
• Sources/TerminalNotificationStore.swift — specifically the playSuppressedNotificationFeedback method

Important Files Changed

Sequence Diagram

sequenceDiagram
    participant T as Terminal
    participant NS as TerminalNotificationStore
    participant AF as AppFocusState
    participant SFH as suppressedFeedbackHandler
    participant NDH as notificationDeliveryHandler
    participant NSS as NotificationSoundSettings
    participant UN as UNNotificationCenter

    T->>NS: addNotification(tabId, surfaceId, ...)
    NS->>AF: isAppFocused() + isFocusedPanel?
    AF-->>NS: shouldSuppressExternalDelivery

    alt shouldSuppressExternalDelivery == true (app focused on that terminal)
        NS->>SFH: suppressedNotificationFeedbackHandler(store, notification)
        SFH->>NSS: playSelectedSound()
        NSS->>NSS: playSound(value, defaults)
        Note over NSS: retainActivePlaybackSound(sound)<br/>sound.delegate = activePlaybackSoundDelegate<br/>sound.play()
        Note right of SFH: ⚠️ runCustomCommand NOT called here
    else shouldSuppressExternalDelivery == false
        NS->>NDH: notificationDeliveryHandler(store, notification)
        NDH->>NS: scheduleUserNotification(notification)
        NS->>UN: center.add(request)
        UN-->>NS: success callback
        NS->>NSS: runCustomCommand(title, subtitle, body)
    end

_{Last reviewed commit: "fix: play notificati..."}